Fix ambiguous for TwoWire::requestFrom() methods and align API with Arduino.cc #8817

safocl · 2023-10-30T07:43:23Z

Description of Change

Remove (comment out) the TwoWire::requestFrom() method overloads.
The compiler says "warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second"

Tests scenarios

Example 1
Example 2
Example 3
All compilers report this warning.

Related links

C++17 standard says ([over.match.best]):

If there is exactly one viable function that is a better function than all other viable functions, then it is the one selected by overload resolution; otherwise the call is ill-formed.) [ Example:

void Fcn(const int*,  short);
void Fcn(int*, int);

int i;
short s = 0;

void f() {
  Fcn(&i, s);       // is ambiguous because &i → int* is better than &i → const int*
                    // but s → short is also better than s → int

  Fcn(&i, 1L);      // calls Fcn(int*, int), because &i → int* is better than &i → const int*
                    // and 1L → short and 1L → int are indistinguishable

  Fcn(&i, 'c');     // calls Fcn(int*, int), because &i → int* is better than &i → const int*
                    // and c → int is better than c → short
}
 — end example ]

There is no viable override for these methods that is better - any overrides require equal conversions of different arguments.

upd: this pull request now reflects the accumulated changes for all errors and warnings about ambiguous TwoWire method overloads. It was decided to make a HardwareI2C interface from Arduino.

CLAassistant · 2023-10-30T07:43:30Z

All committers have signed the CLA.

safocl · 2023-10-30T08:09:42Z

This is an error, not a warning: g++ requires the -pedantic option to conform to the standard.

Inside the function, this overload truncated the data type to a shorter one. This could break some users' hopes.

safocl · 2023-10-30T08:57:48Z

I believe that additional method overloads are not the best solution for disambiguating overload resolution. I believe that requests #5764 and #5768 are erroneous. -- That requests are not solve issue arduino-libraries/ArduinoECCX08#25

VojtechBartoska · 2023-11-01T09:12:11Z

Thanks for the PR, same here please sign CLA.

safocl · 2023-11-01T12:07:26Z

Thanks for the PR, same here please sign CLA.

Oh yes. I didn’t understand at first that this needed to be done.
I did it.

- implement proposal espressif#8818 (comment) to bring the HARDWARE interface into compliance

github-actions · 2024-01-17T16:41:23Z

	Warnings
⚠️	Some issues found for the commit messages in this PR: the commit message `"Fix TwoWire::end() return type."`: summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message `"Fix TwoWire::setClock() return type."`: summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message `"Fix ambiguous for TwoWire::requestFrom() methods."`: summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message `"Fix no return statement in the TwoWire::requestFrom."`: summary should not end with a period (full stop) summary looks empty type/action looks empty the commit message `"Remove TwoWire::begin(int) overload"`: body's lines must not be longer than 100 characters summary looks empty type/action looks empty body must have leading blank line the commit message `"Rewrite TwoWire with using HardwareI2C"`: footer must have leading blank line summary looks empty type/action looks empty the commit message `"fix(libraries/Wire): fix bad return-statement"`: scope/component should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` the commit message `"fix(libraries/Wire): fix return type"`: summary* looks too short scope/component should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` the commit message `"fix(libraries/Wire): hide slave support elements"`: scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` body must have leading blank line the commit message `"fix(libraries/Wire): restore an accidentally deleted implementation"`: scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` body must have leading blank line the commit message `"refactor(libraries/Wire): add return statement if slave isn't supported"`: scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` the commit message `"refactor(libraries/Wire): fix indentation"`: summary* looks too short scope/component should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` the commit message `"refactor(libraries/Wire): fix indentation"`: summary* looks too short scope/component should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` the commit message `"refactor(libraries/Wire): fix indentation"`: summary* looks too short scope/component should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` the commit message `"refactor(libraries/Wire): remove temporary comment"`: scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` the commit message `"refactor(libraries/Wire): remove unnecessary empty lines"`: scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` the commit message `"refactor(libraries/Wire): remove unused variables"`: body's lines must not be longer than 100 characters scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` body must have leading blank line the commit message `"refactor(libraries/Wire): remove unused variables"`: body's lines must not be longer than 100 characters scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` body must have leading blank line the commit message `"refactor(libraries/Wire): return return types"`: summary* looks too short scope/component should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` body must have leading blank line the commit message `"refactor(libraries/Wire): use slave without support"`: scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` body must have leading blank line the commit message `"style(libraries/Wire): replace tabs with spaces"`: scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` type/action* should be one of [`change`, `ci`, `docs`, `feat`, `fix`, `refactor`, `remove`, `revert`, `test`] Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: `<type/action>(<scope/component>): <summary>`, for example `fix(esp32): Fixed startup timeout issue` allowed types are: `change,ci,docs,feat,fix,refactor,remove,revert,test` sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) `TIP:` Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).
⚠️	Please consider squashing your 27 commits (simplifying branch history).
⚠️	The source branch `"TwoWireInheritFromHardwareI2C"` incorrect format: contains uppercase letters. This can cause troubles on case-insensitive file systems (macOS). Please rename your branch.

👋 Hello safocl, we appreciate your contribution to this project!

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 311330e

lucasssvaz · 2024-01-17T16:49:50Z

@safocl Please fix the CI errors

Remove non-void values of the return-statements in function returning 'void'.

TwoWire::begin(uint8_t address) should be available without slave support by SoC?

safocl · 2024-01-18T01:27:41Z

@safocl Please fix the CI errors

I made commits.
Need review for TwoWire::begin(uint8_t address) -- it is should be available without slave
support by SoC?

Compiler reports "Wire.cpp:393:15: error: variable 'err' set but not used [-Werror=unused-but-set-variable]".

lucasssvaz

I don't think we should remove the error checking from the function return. It is a good way to check if everything inside the method was correctly checked.

The aim of this PR is fixing the ambiguity of the requestFrom methods. I think it is fine to create a HardwareI2C class here but removing the return values should be done in another PR so it can be discussed.

TwoWire::endTransmission() was accidentally deleted from a cpp file.

lucasssvaz

After discussing with the team we agreed that it is better to keep the return types in the abstract class and the implementation even if differ from the Arduino.cc API.

safocl · 2024-01-23T05:13:44Z

After discussing with the team we agreed that it is better to keep the return types in the abstract class and the implementation even if differ from the Arduino.cc API.

Do I need to return the return types as they were originally and change them in the abstract class itself?

me-no-dev · 2024-01-23T10:40:04Z

Yes. change HardwareI2C and revert in Wire

In TwoWire class return return types.

P-R-O-C-H-Y

@safocl PTAL

libraries/Wire/src/Wire.h

Co-authored-by: Jan Procházka <[email protected]>

P-R-O-C-H-Y

Just small formatting fixes, else looks good.
Tested i2c with OLED, works fine.

libraries/Wire/src/Wire.cpp

libraries/Wire/src/Wire.h

Co-authored-by: Jan Procházka <[email protected]>

P-R-O-C-H-Y

Thanks @safocl, great job!

Fix ambiguous for TwoWire::requestFrom() methods.

cba06d1

Remove TwoWire::begin(int) overload

14660ef

Inside the function, this overload truncated the data type to a shorter one. This could break some users' hopes.

VojtechBartoska added the Status: Review needed Issue or PR is awaiting review label Nov 1, 2023

VojtechBartoska requested review from me-no-dev and P-R-O-C-H-Y November 1, 2023 09:11

Rewrite TwoWire with using HardwareI2C

0454e3a

- implement proposal espressif#8818 (comment) to bring the HARDWARE interface into compliance

VojtechBartoska added the Status: Needs investigation We need to do some research before taking next steps on this issue label Nov 28, 2023

safocl added 4 commits November 28, 2023 17:11

Merge branch 'master' into TwoWireInheritFromHardwareI2C

c7a3b6e

Fix TwoWire::end() return type.

b36ff08

Fix TwoWire::setClock() return type.

f294609

Fix no return statement in the TwoWire::requestFrom.

a1e2e69

safocl mentioned this pull request Nov 28, 2023

Remove TwoWire::begin(int) overload #8818

Closed

VojtechBartoska added this to the 3.0.0 milestone Nov 29, 2023

VojtechBartoska requested a review from lucasssvaz January 17, 2024 13:23

lucasssvaz added the hil_test Run Hardware Tests label Jan 17, 2024

Merge branch 'master' into TwoWireInheritFromHardwareI2C

70a5316

safocl added 4 commits January 18, 2024 03:54

Merge branch 'espressif:master' into TwoWireInheritFromHardwareI2C

b2ad191

fix(libraries/Wire): fix bad return-statement

fca0f85

Remove non-void values of the return-statements in function returning 'void'.

style(libraries/Wire): replace tabs with spaces

135ad6a

refactor(libraries/Wire): use slave without support

3cf20e0

TwoWire::begin(uint8_t address) should be available without slave support by SoC?

safocl added 2 commits January 18, 2024 16:52

Merge branch 'espressif:master' into TwoWireInheritFromHardwareI2C

e56d262

refactor(libraries/Wire): remove unused variables

7a351dd

Compiler reports "Wire.cpp:393:15: error: variable 'err' set but not used [-Werror=unused-but-set-variable]".

lucasssvaz requested changes Jan 18, 2024

View reviewed changes

safocl added 3 commits January 19, 2024 23:48

Merge branch 'espressif:master' into TwoWireInheritFromHardwareI2C

a787d49

refactor(libraries/Wire): remove temporary comment

1b9f656

fix(libraries/Wire): restore an accidentally deleted implementation

75f8ee9

TwoWire::endTransmission() was accidentally deleted from a cpp file.

lucasssvaz approved these changes Jan 22, 2024

View reviewed changes

lucasssvaz changed the title ~~Fix ambiguous for TwoWire::requestFrom() methods.~~ Fix ambiguous for TwoWire::requestFrom() methods and align API with Arduino.cc Jan 22, 2024

lucasssvaz requested changes Jan 22, 2024

View reviewed changes

lucasssvaz added Resolution: Awaiting response Waiting for response of author and removed Status: Needs investigation We need to do some research before taking next steps on this issue labels Jan 26, 2024

safocl added 3 commits January 28, 2024 21:07

Merge branch 'espressif:master' into TwoWireInheritFromHardwareI2C

5d3c8c7

refactor(libraries/Wire): return return types

07154af

In TwoWire class return return types.

fix(libraries/Wire): fix return type

69a5890

P-R-O-C-H-Y requested changes Jan 29, 2024

View reviewed changes

libraries/Wire/src/Wire.h Outdated Show resolved Hide resolved

refactor(libraries/Wire): add return statement if slave isn't supported

35c0568

Co-authored-by: Jan Procházka <[email protected]>

P-R-O-C-H-Y requested changes Jan 29, 2024

View reviewed changes

libraries/Wire/src/Wire.cpp Outdated Show resolved Hide resolved

libraries/Wire/src/Wire.cpp Outdated Show resolved Hide resolved

libraries/Wire/src/Wire.cpp Outdated Show resolved Hide resolved

libraries/Wire/src/Wire.h Outdated Show resolved Hide resolved

lucasssvaz approved these changes Jan 29, 2024

View reviewed changes

safocl and others added 4 commits January 29, 2024 16:49

refactor(libraries/Wire): fix indentation

eacea66

Co-authored-by: Jan Procházka <[email protected]>

refactor(libraries/Wire): fix indentation

b1ca983

Co-authored-by: Jan Procházka <[email protected]>

refactor(libraries/Wire): fix indentation

bca2887

Co-authored-by: Jan Procházka <[email protected]>

refactor(libraries/Wire): remove unnecessary empty lines

311330e

P-R-O-C-H-Y approved these changes Jan 29, 2024

View reviewed changes

P-R-O-C-H-Y added Status: Pending Merge Pull Request is ready to be merged and removed Resolution: Awaiting response Waiting for response of author Status: Review needed Issue or PR is awaiting review labels Jan 29, 2024

VojtechBartoska modified the milestones: 3.0.0, 3.0.0-RC1 Jan 30, 2024

me-no-dev merged commit 7966f4a into espressif:master Jan 31, 2024
136 of 139 checks passed

Staars mentioned this pull request Feb 2, 2024

Berry Wire: fix compilation with coming update of Arduino framework arendst/Tasmota#20641

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ambiguous for TwoWire::requestFrom() methods and align API with Arduino.cc #8817

Fix ambiguous for TwoWire::requestFrom() methods and align API with Arduino.cc #8817

safocl commented Oct 30, 2023 •

edited

Loading

CLAassistant commented Oct 30, 2023 •

edited

Loading

safocl commented Oct 30, 2023

safocl commented Oct 30, 2023

VojtechBartoska commented Nov 1, 2023

safocl commented Nov 1, 2023

github-actions bot commented Jan 17, 2024 •

edited

Loading

lucasssvaz commented Jan 17, 2024

safocl commented Jan 18, 2024 •

edited

Loading

lucasssvaz left a comment

lucasssvaz left a comment

safocl commented Jan 23, 2024

me-no-dev commented Jan 23, 2024

P-R-O-C-H-Y left a comment

P-R-O-C-H-Y left a comment

P-R-O-C-H-Y left a comment

Fix ambiguous for TwoWire::requestFrom() methods and align API with Arduino.cc #8817

Fix ambiguous for TwoWire::requestFrom() methods and align API with Arduino.cc #8817

Conversation

safocl commented Oct 30, 2023 • edited Loading

Description of Change

Tests scenarios

Related links

CLAassistant commented Oct 30, 2023 • edited Loading

safocl commented Oct 30, 2023

safocl commented Oct 30, 2023

VojtechBartoska commented Nov 1, 2023

safocl commented Nov 1, 2023

github-actions bot commented Jan 17, 2024 • edited Loading

lucasssvaz commented Jan 17, 2024

safocl commented Jan 18, 2024 • edited Loading

lucasssvaz left a comment

Choose a reason for hiding this comment

lucasssvaz left a comment

Choose a reason for hiding this comment

safocl commented Jan 23, 2024

me-no-dev commented Jan 23, 2024

P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

safocl commented Oct 30, 2023 •

edited

Loading

CLAassistant commented Oct 30, 2023 •

edited

Loading

github-actions bot commented Jan 17, 2024 •

edited

Loading

safocl commented Jan 18, 2024 •

edited

Loading